Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

box2d kengz vs py #1120

Merged
merged 13 commits into from
Aug 9, 2018
Merged

box2d kengz vs py #1120

merged 13 commits into from
Aug 9, 2018

Conversation

pzhokhov
Copy link
Collaborator

@pzhokhov pzhokhov commented Aug 6, 2018

No description provided.

@pzhokhov pzhokhov mentioned this pull request Aug 6, 2018
Closed
christopherhesse
christopherhesse requested changes Aug 7, 2018
View reviewed changes
Copy link
Contributor

@christopherhesse christopherhesse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure we can't just have a single box2d that works on mac and linux? It looks like box2d-py was made by openai to fix this, could we fix box2d-py to work on mac? https://pypi.org/project/box2d-py/

@pzhokhov
Copy link
Collaborator Author

pzhokhov commented Aug 7, 2018

The gist of the problem is follows - the pybox2d build command (the one that builds libraries) is not actually being executed when installing from pypi (specifically, python setup.py bdist_wheel does not trigger it, but python setup.py build or python setup.py develop does; I think related to pypa/setuptools#1194).
To solve it, pybox2d can be installed from source like this:
pip install -e git+git://github.com/pybox2d/pybox2d.git@3818cf5#egg=Box2D
I'll add respective comment to readme and change the logic in the container, but I am afraid it is about as much as we can do without sinking a ton of time into messing with setuptools logic.

@christopherhesse
Copy link
Contributor

Well, if we're unwilling to update our custom python package for box2d, could we have setup.py dynamically depend on box2d-py or the other box2d based on platform.system(), or does that not work? Adding a note to the README puts the work on the user who will probably just think our code is broken since it doesn't work even after following the install instructions.

@pzhokhov
Copy link
Collaborator Author

pzhokhov commented Aug 7, 2018

Fair point... We can definitely do the latter, and let me spend another half-hour on trying to fix the pybox2d or box2d-py package - maybe there is some simple solution after all.

@christopherhesse
Copy link
Contributor

One difference is that box2d-py is source only, while box2d-kengz has a wheel for mac: https://pypi.org/project/Box2D-kengz/#files

@christopherhesse
Copy link
Contributor

Compiling a wheel for mac with python setup.py bdist_wheel seems to be broken with pybox2d and box2d-py, but box2d-kengz works, available here: https://files.pythonhosted.org/packages/81/20/51d6c0c87f7642efb709c518fb0ca8e5eab068259588552c41da5926ae27/Box2D-kengz-2.3.3.tar.gz

But of course, that one seems to fail on linux, while box2d-py works, so updating box2d-py to contain any sort of fix from box2d-kengz could be a pretty good fix.

@pzhokhov
Copy link
Collaborator Author

pzhokhov commented Aug 7, 2018

yeah, that's what I have been thinking too, comparing them now

@pzhokhov
Copy link
Collaborator Author

pzhokhov commented Aug 7, 2018

ugh it looks like bdist_wheel it is actually broken on both; but python file that is exported from swig interface has a version compatible with osx in box2d-kengz and the version compatible with linux in box2d-py :( hence one works in one case and the other - in the other case

@christopherhesse
Copy link
Contributor

Yeah, that's kinda gross. It looks like gym-retro overrides build_ext, does that let you execute the correct build steps on install? https://github.com/openai/gym-retro/blob/master/setup.py#L80

@pzhokhov
Copy link
Collaborator Author

pzhokhov commented Aug 7, 2018

Yeah, the fix I was thinking of is also along these lines, but rather to override install so that it copies newly generated *.py files to where they need to go (the correct files are generated, but *.py files are copied before building, so the correct files end up never being used)

@pzhokhov
Copy link
Collaborator Author

pzhokhov commented Aug 7, 2018

whew I think I fixed it, now we just need to release the pypi package

@christopherhesse
Copy link
Contributor

Nice!

@christopherhesse
Copy link
Contributor

Are we going to have some fork of pybox2d at like openai/box2d-py?

@pzhokhov
Copy link
Collaborator Author

pzhokhov commented Aug 7, 2018

oh, and now I see how the pybox2d CI works - they use swig3.0 instead of default swig :/ Still looks ugly to me.

@pzhokhov
Copy link
Collaborator Author

pzhokhov commented Aug 7, 2018

openai/box2d-py#1 (for some reason I cannot add @christopherhesse as a reviewer)

@christopherhesse
Copy link
Contributor

Nice, are we dropping py27 support here as well?

@pzhokhov
Copy link
Collaborator Author

pzhokhov commented Aug 8, 2018

Well... It was not actually tested with py27 and I am pretty sure tests won't work with py27 (like some extra magic is needed to make that work). I thought it is being at the end of its life, but, apparently, not: https://legacy.python.org/dev/peps/pep-0373/
If we want to support it, we need to make sure tests run with py27.

@pzhokhov
Copy link
Collaborator Author

pzhokhov commented Aug 8, 2018

yeah, there are failures like

>       model = mujoco_py.load_model_from_path(fullpath)
E       AttributeError: 'module' object has no attribute 'load_model_from_path'

with python2.7 (apparently, related to openai/mujoco-py#261).
I think we should drop support of py27 for now, and create a separate issue for that.

@pzhokhov
Copy link
Collaborator Author

pzhokhov commented Aug 9, 2018

@christopherhesse are you ok with merging this?

@christopherhesse
Copy link
Contributor

LGTM!

@pzhokhov pzhokhov merged commit c306b4e into master Aug 9, 2018
@pzhokhov pzhokhov deleted the peterz_box2d_kengz_vs_py branch August 9, 2018 17:13
gabrielnan pushed a commit to ACDSLab/gym that referenced this pull request Aug 15, 2018
@pzhokhov @gabrielnan
box2d kengz vs py (openai#1120)
1332bbf 
* try build with box2d-kengz

* test box2d envs

* use box2d-py in tox.ini for tests

* box2d-py instead of box2d-kengz in box2d dependencies

* test dependencies in tox.ini use dependencies in setup.py

* further cleanups of tox.ini

* further cleanups

* added scipy to list of requirements

* added a note about box2d-kengz into README

* build box2d from scratch, add a note to README about it

* use box2d-py>=2.3.4

* removed box2d installation instructions from README
naeioi added a commit to rlworkgroup/garage that referenced this pull request Oct 8, 2018
@naeioi
Fix Box2D in fresh install (#334)
16a7c2a 
Before box2d-py 2.3.4, box2d-py works on Linux but breaks on OSX, and
Box2D-kengz works on OSX but breaks on Linux. box2d-py 2.3.4 fixes issue
on OSX and now works on both platforms, and is actively by OpenAI.

See openai/gym#1120.
naeioi added a commit to rlworkgroup/garage that referenced this pull request Oct 15, 2018
@naeioi
Fix Box2D in fresh install (#334)
4709c24 
Before box2d-py 2.3.4, box2d-py works on Linux but breaks on OSX, and
Box2D-kengz works on OSX but breaks on Linux. box2d-py 2.3.4 fixes issue
on OSX and now works on both platforms, and is actively by OpenAI.

See openai/gym#1120.
naeioi added a commit to rlworkgroup/garage that referenced this pull request Oct 17, 2018
@naeioi
Fix Box2D in fresh install (#334)
9726135 
Before box2d-py 2.3.4, box2d-py works on Linux but breaks on OSX, and
Box2D-kengz works on OSX but breaks on Linux. box2d-py 2.3.4 fixes issue
on OSX and now works on both platforms, and is actively by OpenAI.

See openai/gym#1120.
ryanjulian pushed a commit to rlworkgroup/garage that referenced this pull request Oct 21, 2018
@naeioi @ryanjulian
Fix Box2D in fresh install (#334)
a45e99a 
Before box2d-py 2.3.4, box2d-py works on Linux but breaks on OSX, and
Box2D-kengz works on OSX but breaks on Linux. box2d-py 2.3.4 fixes issue
on OSX and now works on both platforms, and is actively by OpenAI.

See openai/gym#1120.
naeioi added a commit to rlworkgroup/garage that referenced this pull request Oct 23, 2018
@naeioi
Fix Box2D in fresh install (#334)
829a27f 
Before box2d-py 2.3.4, box2d-py works on Linux but breaks on OSX, and
Box2D-kengz works on OSX but breaks on Linux. box2d-py 2.3.4 fixes issue
on OSX and now works on both platforms, and is actively by OpenAI.

See openai/gym#1120.
naeioi added a commit to rlworkgroup/garage that referenced this pull request Oct 25, 2018
@naeioi
Fix broken Box2D dependency in fresh install (#335)
085a35d 
* Fix Box2D in fresh install (#334)

Before box2d-py 2.3.4, box2d-py works on Linux but breaks on OSX, and
Box2D-kengz works on OSX but breaks on Linux. box2d-py 2.3.4 fixes issue
on OSX and now works on both platforms, and is actively by OpenAI.

See openai/gym#1120.

* Cleanup (#334)

* Cleanup extra pip (#334)

Remove pip box2d-py in
- setup_linux.sh
- setup_osx.sh
- Dockerfile.ci

* Upgrade gym to 0.10.8 (#334)

* Remove assertion for Mujoco viewer close

This problem is resolved in #340

* Close all env

* Remove deadcode KNOWN_GYM_CLOSE_BROKEN
akumaraguru pushed a commit to rlworkgroup/garage that referenced this pull request Oct 26, 2018
@naeioi @akumaraguru
Fix broken Box2D dependency in fresh install (#335)
b0c91ce 
* Fix Box2D in fresh install (#334)

Before box2d-py 2.3.4, box2d-py works on Linux but breaks on OSX, and
Box2D-kengz works on OSX but breaks on Linux. box2d-py 2.3.4 fixes issue
on OSX and now works on both platforms, and is actively by OpenAI.

See openai/gym#1120.

* Cleanup (#334)

* Cleanup extra pip (#334)

Remove pip box2d-py in
- setup_linux.sh
- setup_osx.sh
- Dockerfile.ci

* Upgrade gym to 0.10.8 (#334)

* Remove assertion for Mujoco viewer close

This problem is resolved in #340

* Close all env

* Remove deadcode KNOWN_GYM_CLOSE_BROKEN
ryanjulian pushed a commit to rlworkgroup/metaworlds that referenced this pull request Feb 23, 2019
@naeioi
Fix broken Box2D dependency in fresh install (#335)
8e5d4a0 
* Fix Box2D in fresh install (#334)

Before box2d-py 2.3.4, box2d-py works on Linux but breaks on OSX, and
Box2D-kengz works on OSX but breaks on Linux. box2d-py 2.3.4 fixes issue
on OSX and now works on both platforms, and is actively by OpenAI.

See openai/gym#1120.

* Cleanup (#334)

* Cleanup extra pip (#334)

Remove pip box2d-py in
- setup_linux.sh
- setup_osx.sh
- Dockerfile.ci

* Upgrade gym to 0.10.8 (#334)

* Remove assertion for Mujoco viewer close

This problem is resolved in #340

* Close all env

* Remove deadcode KNOWN_GYM_CLOSE_BROKEN
ryanjulian pushed a commit to rlworkgroup/metaworlds that referenced this pull request Feb 23, 2019
@naeioi
Fix broken Box2D dependency in fresh install (#335)
f2f0f14 
* Fix Box2D in fresh install (#334)

Before box2d-py 2.3.4, box2d-py works on Linux but breaks on OSX, and
Box2D-kengz works on OSX but breaks on Linux. box2d-py 2.3.4 fixes issue
on OSX and now works on both platforms, and is actively by OpenAI.

See openai/gym#1120.

* Cleanup (#334)

* Cleanup extra pip (#334)

Remove pip box2d-py in
- setup_linux.sh
- setup_osx.sh
- Dockerfile.ci

* Upgrade gym to 0.10.8 (#334)

* Remove assertion for Mujoco viewer close

This problem is resolved in #340

* Close all env

* Remove deadcode KNOWN_GYM_CLOSE_BROKEN
ryanjulian pushed a commit to rlworkgroup/metaworlds that referenced this pull request Feb 23, 2019
@naeioi
Fix broken Box2D dependency in fresh install (#335)
426afa7 
* Fix Box2D in fresh install (#334)

Before box2d-py 2.3.4, box2d-py works on Linux but breaks on OSX, and
Box2D-kengz works on OSX but breaks on Linux. box2d-py 2.3.4 fixes issue
on OSX and now works on both platforms, and is actively by OpenAI.

See openai/gym#1120.

* Cleanup (#334)

* Cleanup extra pip (#334)

Remove pip box2d-py in
- setup_linux.sh
- setup_osx.sh
- Dockerfile.ci

* Upgrade gym to 0.10.8 (#334)

* Remove assertion for Mujoco viewer close

This problem is resolved in #340

* Close all env

* Remove deadcode KNOWN_GYM_CLOSE_BROKEN
ryanjulian pushed a commit to rlworkgroup/metaworlds that referenced this pull request Feb 23, 2019
@naeioi
Fix broken Box2D dependency in fresh install (#335)
a0cb7ee 
* Fix Box2D in fresh install (#334)

Before box2d-py 2.3.4, box2d-py works on Linux but breaks on OSX, and
Box2D-kengz works on OSX but breaks on Linux. box2d-py 2.3.4 fixes issue
on OSX and now works on both platforms, and is actively by OpenAI.

See openai/gym#1120.

* Cleanup (#334)

* Cleanup extra pip (#334)

Remove pip box2d-py in
- setup_linux.sh
- setup_osx.sh
- Dockerfile.ci

* Upgrade gym to 0.10.8 (#334)

* Remove assertion for Mujoco viewer close

This problem is resolved in #340

* Close all env

* Remove deadcode KNOWN_GYM_CLOSE_BROKEN
zlig pushed a commit to zlig/gym that referenced this pull request Apr 24, 2020
@pzhokhov
box2d kengz vs py (openai#1120)
e72764c 
* try build with box2d-kengz

* test box2d envs

* use box2d-py in tox.ini for tests

* box2d-py instead of box2d-kengz in box2d dependencies

* test dependencies in tox.ini use dependencies in setup.py

* further cleanups of tox.ini

* further cleanups

* added scipy to list of requirements

* added a note about box2d-kengz into README

* build box2d from scratch, add a note to README about it

* use box2d-py>=2.3.4

* removed box2d installation instructions from README
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christopherhesse christopherhesse christopherhesse approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pzhokhov @christopherhesse